Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve module augmentation in table plugin #13568

Merged

Conversation

filipsobol
Copy link
Member

@filipsobol filipsobol commented Feb 27, 2023

Suggested merge commit message (convention)

Internal: Improve TypeScript configuration in the project and module augmentation in table plugin. Related to #13565.

packages/ckeditor5-table/package.json Outdated Show resolved Hide resolved
packages/ckeditor5-table/plugin.d.ts Outdated Show resolved Hide resolved
packages/ckeditor5-table/plugin.d.ts Outdated Show resolved Hide resolved
packages/ckeditor5-table/tsconfig.json Outdated Show resolved Hide resolved
packages/ckeditor5-table/plugin.d.ts Outdated Show resolved Hide resolved
@niegowski
Copy link
Contributor

I'm also concerned about automatic tests for other packages. Those import other features to test integrations and they probably do not import using index files. Those could fail if augmentations are not provided.

@Reinmar
Copy link
Member

Reinmar commented Feb 28, 2023

My biggest doubt regarding this is: why do we have plugin.d.ts and index.ts? Why not merging them into one file? And keeping there all the module augmentation too?

@filipsobol
Copy link
Member Author

We decided to keep all module augmentation in a separate file (not inlined in index.ts) to be able to include it in the tsconfig.json (link) and tsconfig.release.json (link), so that module augmentation works during the development and build process.

This doesn't have to be applied in the consuming projects as long as they import everything from the main index.ts file, eg.:

// ✔️
import { Table } from '@ckeditor/ckeditor5-table';

// ❌
import Table from '@ckeditor/ckeditor5-table/src/table';

@Reinmar
Copy link
Member

Reinmar commented Feb 28, 2023

  • Let's not expose commands in index.js so far. See one of the above comments.
  • Let's not expose stuff like MouseEventsObserver from index.js just to satisfy tests.
  • Revert the import-related changes in tests of other packages. Instead, try to change tsconfig.json to use a glob pattern like ../../packages/*/src/augmentation.ts + ../../external/*/packages/*/src/augmentation.ts to make sure that all augmentations are loaded in the test env.

@Reinmar
Copy link
Member

Reinmar commented Feb 28, 2023

Note: Squash all commits in this branch before the merge so to not pollute the history with all the test import changes.

@filipsobol
Copy link
Member Author

Adding "./packages/*/src/augmentation.ts" to include array in tsconfig.json didn't work when running testautomated.js. However, it worked when we added "./packages/ckeditor5-table/src/augmentation.ts" to files.

When debugging this with @niegowski and comparing behavior of tsc and testautomated.js, we found that tsc worked with both configurations. After we enabled the logLevel: 'info' option in ts-loader we noticed that it loads tsconfig.js for paragraph plugin, despite testing clipboard plugin in root of the monorepo project. When we added "./packages/*/src/augmentation.ts" to include array in ./packages/ckeditor5-paragraph/tsconfig.json, it started working as expected for clipboard.

We need to investigate why seemingly random tsconfig.json is being loaded by ts-loader. For now, as a quick workaround, we will add "./packages/*/src/augmentation.ts"to include arrays in all packages.

@filipsobol
Copy link
Member Author

Above issue with the wrong `tsconfig.json` being loaded will be fixed in ckeditor/ckeditor5-dev#847

@filipsobol filipsobol force-pushed the ck/13565-improving-module-augmentation-for-table-plugin branch from 26325f6 to 66e9eb2 Compare March 1, 2023 16:06
@filipsobol filipsobol force-pushed the ck/13565-improving-module-augmentation-for-table-plugin branch from e368dd8 to c073e28 Compare March 1, 2023 16:08
@Reinmar
Copy link
Member

Reinmar commented Mar 1, 2023

Note to self: I wanted to comment that you changed formatting of JSON files and I remember that we have a special rule for them in .editorconfig to use two spaces. But then I checked it's for package.jsons only :D And we actually use tabs in other JSON files. Case closed :D

@filipsobol filipsobol force-pushed the ck/13565-improving-module-augmentation-for-table-plugin branch from 02959b2 to 6166b55 Compare March 2, 2023 11:42
scripts/check-manual-tests.sh Outdated Show resolved Hide resolved
scripts/check-manual-tests.sh Outdated Show resolved Hide resolved
@filipsobol filipsobol force-pushed the ck/13565-improving-module-augmentation-for-table-plugin branch from 351e81f to dd39b3c Compare March 2, 2023 14:29
@filipsobol filipsobol requested a review from niegowski March 2, 2023 14:30
@niegowski
Copy link
Contributor

Please update the "Suggested merge commit message" to address changes in this PR.

tsconfig.test.json Outdated Show resolved Hide resolved
@filipsobol filipsobol force-pushed the ck/13565-improving-module-augmentation-for-table-plugin branch from 2c85bd5 to 0f1b3b0 Compare March 3, 2023 08:42
@filipsobol filipsobol requested a review from pomek March 3, 2023 09:15
scripts/check-manual-tests.sh Outdated Show resolved Hide resolved
Copy link
Member

@pomek pomek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. But TBH, too many changes. I will depend on CI results.

@filipsobol filipsobol force-pushed the ck/13565-improving-module-augmentation-for-table-plugin branch from 84478c6 to bd77486 Compare March 3, 2023 09:48
@niegowski niegowski merged commit eddb4e8 into master Mar 3, 2023
@niegowski niegowski deleted the ck/13565-improving-module-augmentation-for-table-plugin branch March 3, 2023 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants